Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pool: Reuse unknownFields memory #143

Closed
wants to merge 1 commit into from

Conversation

codesoap
Copy link
Contributor

@codesoap codesoap commented Oct 3, 2024

First of all: Thanks for the great project! I really like the approach and have seen great performance improvements with vtprotobuf.

While analysing the memory usage of my program using vtprotobuf, I noticed that massive amounts of memory are used for the allocation of unknownFields. This is a big part of my data, as I am parsing some existing protobuf (open streetmap's PBF files) where I don't care about a lot of the fields, so I have excluded them from my .proto files to increase performance. Reusing the memory of the unknownFields reduced the amount of allocations by 34% and the amount of used memory by 41% in my case.

I'm pretty sure this PR is in no shape to be merged, because I still understand too little about the code to write it better, but thought a working PR would demonstrate my idea better than an issue.

@codesoap
Copy link
Contributor Author

codesoap commented Oct 3, 2024

Btw. it's not clear to me why unknownFields is necessary at all. Is it an option to just leave it empty? In this case, we wouldn't need to bother with this PR.

@vmg
Copy link
Member

vmg commented Oct 7, 2024

Hey! Glad to see you find the library useful. unknownFields are definitely not necessary to parse, we could skip them on a per-message basis if you just don't care about them. I would happily merge that PR.

You could start by adding an ignore_unknown_fields flag to our extensions proto: https://github.com/planetscale/vtprotobuf/blob/main/include/github.com/planetscale/vtprotobuf/vtproto/ext.proto and then reading it like we do for the mempool flag. Let me know how that goes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants